-
Notifications
You must be signed in to change notification settings - Fork 13.5k
make cfg_select
a builtin macro
#143461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
make cfg_select
a builtin macro
#143461
Conversation
This comment has been minimized.
This comment has been minimized.
e3aeb08
to
1dc0034
Compare
#[instrument(skip(cx, tts))] | ||
pub fn expand_token_stream<'cx>( | ||
cx: &'cx mut ExtCtxt<'_>, | ||
sp: Span, | ||
arm_span: Span, | ||
node_id: NodeId, | ||
name: Ident, | ||
tts: TokenStream, | ||
) -> Box<dyn MacResult + 'cx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to know if there is already a standard way to expand a token stream. I couldn't find one, so I did some refactoring here.
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. This PR modifies cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior, per the tests and the impl, looks correct to me.
3d68353
to
3abcece
Compare
I moved some things around (analogously to Implementation-wise the one thing I'm not sure about is how to get |
cc @rust-lang/rustfmt @rust-lang/style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the relevant file for rustfmt
: the parse_cfg_select
function returns a CfgSelectBranches
that keeps (as far as I can tell) all of the structure that is required to format cfg_select
.
A cfg_select
looks like this
fn print() {
println!(cfg_select! {
unix => { "unix" }
_ => { "not unix" }
});
}
where the right-hand side of the arrow is a TokenTree
. That token tree is expanded based on the context: if the macro is in expression position, it'll be parsed as an expression, similarly for statements, items and any other position where a macro can occur.
So the formatter heeds to handle this TokenTree
(really a TokenStream
after we strip of the outer braces) somehow. I didn't see immediately how to do that given the existing APIs, but I assume it's possible because macros-by-example would need the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[derive(Default)]
pub struct CfgSelectBranches {
pub reachable: Vec<(MetaItemInner, TokenStream, Span)>,
pub wildcard: Option<(Token, TokenStream, Span)>,
pub unreachable: Vec<(CfgSelectRule, TokenStream, Span)>,
}
Question, what's the difference between reachable
, wildcard
, and unreachable
?
Also, will the TokenTree
on the right-hand side of the arrow always be valid rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, what's the difference between
reachable
,wildcard
, andunreachable
?
For formatting there is really no difference. The goal of this structure is to be able to emit warnings for unreachable branches.
The cfg rules on the left-hand side are evaluated from top to bottom, and the first one that evaluates to true is picked. The wildcard _
always evaluates to true, so any branches that follow it are unreachable.
Also, will the
TokenTree
on the right-hand side of the arrow always be valid rust?
I don't think so. It is like the right-hand side of a macro_rules!
macro rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide examples of more complicated RHS that aren't valid rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this
error: macro expansion ignores `+` and any tokens following
--> /home/folkertdev/rust/rust/tests/ui/macros/cfg_select.rs:30:12
|
LL | / cfg_select! {
LL | | _ => { + + + }
| | ^
LL | | }
| |_- caused by the macro expansion here
|
= note: the usage of `cfg_select!` is likely invalid in item context
the rhs is a valid token tree, but when expanded it's not valid rust code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustfmt would definitely fail to handle that. Is there a more realistic example you can think of?
In the example above things fail to compile, but is there a case where the RHS is composed of valid tokens that don't parse as valid rust, but still get expanded to valid rust?
Technically, rustfmt doesn't care what the tokens get expanded to as long as the tokens themselves can be parsed as valid rust, otherwise there's no hope to format them. Would you say that the typical case is going to be a RHS that parses as valid rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in valid programs the rhs will generally be valid rust. The only tricky thing is that it's unclear what kind (could be a sequence of items, or an expression, or a statement).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I think it should be enough to call format_snippet
. That's how we currently format branches in macro_rules
macros.
☔ The latest upstream changes (presumably #143521) made this pull request unmergeable. Please resolve the merge conflicts. |
3abcece
to
53b3b88
Compare
☔ The latest upstream changes (presumably #143582) made this pull request unmergeable. Please resolve the merge conflicts. |
53b3b88
to
5d550b1
Compare
4b84ad7
to
01f41cc
Compare
tracking issue: #115585
This parses mostly the same as the
macro cfg_select
version, except:cfg_select {{ /* ... */ }}
is now rejected.cfg_select { _ => 1 }
now worksI've also added an error if none of the rules evaluate to true, and warnings for any arms that follow the
_
wildcard rule.cc @traviscross if I'm missing any feature that should/should not be included
r? @petrochenkov for the macro logic details